feat: use clarity-serialization in clarity
#6310
Conversation
…end-on-clarity-serialization
…end-on-clarity-serialization
…end-on-clarity-serialization
…end-on-clarity-serialization
…end-on-clarity-serialization
…ity-depend-on-clarity-serialization
…ity-depend-on-clarity-serialization
github-project-automation
bot
moved this from Status: In Review
to Status: 💻 In Progress
in Stacks Core Eng
| #[derive(Clone, PartialEq, Eq, Hash, Debug, Serialize, Deserialize)] | ||
| pub struct FunctionIdentifier { | ||
| identifier: String, | ||
| } | ||
|
|
There was a problem hiding this comment.
This is needed for the StackTrace type used in Error
| IOError(IncomparableError<std::io::Error>), | ||
| BadTypeError(CheckErrors), | ||
| DeserializationError(String), | ||
| DeserializeExpected(Box<TypeSignature>), |
There was a problem hiding this comment.
originally, this was DeserializeExpected(TypeSignature) does this change cause any problems?
There was a problem hiding this comment.
I think this is fine, because AFAIK we never match on whether or not a substructure is a Box<T> anywhere in the codebase (that would be very weird).
There was a problem hiding this comment.
Can you explain the reason for the change?
There was a problem hiding this comment.
Same reason as for #6310 (comment). But if you prefer I could move even this change to the separate PR
| @@ -12,11 +12,12 @@ readme = "README.md" | |||
| [dependencies] | |||
| lazy_static = { workspace = true } | |||
| regex = { version = "1", default-features = false } | |||
| rusqlite = { workspace = true, optional = true } | |||
There was a problem hiding this comment.
needed for implementing ToSql and FromSql for ExecutionCost, and defining InterpreterError::SqliteError(IncomparableError<SqliteError>)
There was a problem hiding this comment.
Hmm, this might need some thought. I thought clarity-serialization was supposed to be able to run in WASM? If so, then rusqlite can't be a required dependency IIRC.
There was a problem hiding this comment.
It shouldn't be a problem in this case, rustqlite will only be included if built with the rusqlite feature. If building this with
cargo build -p clarity-serialization --features "wasm-web" (for js support)
cargo build -p clarity-serialization --features "wasm-deterministic" (for deterministic environment like CosmWasm)
it will still work!
There was a problem hiding this comment.
All the structs remaining in this file (e.g. FunctionSignature, FixedFunction, etc.) and their implementations, were not originally needed in clarity-serialization, but since we are going to rename this into clarity-types I was thinking of moving them all. Any opinions?
| } | ||
| } | ||
| } | ||
|
|
||
| /// Parsing functions. | ||
| impl TypeSignature { | ||
| pub trait TypeSignatureExt { |
There was a problem hiding this comment.
I added a comment for now, but I am open to revisit this if you think otherwise! The main reasons I initially avoided moving this in clarity-serialization was because most of these functions require both for SymbolicExpression and CostTracker which I saw more as a runtime construct and more appropriate for clarity. CostTracker also depends on ClarityCostFunction, ExecutionCost, and CostErrors.
But with the latest changes I already needed to move ExecutionCost and CostErrors into clarity-serialization, so I’m open to also moving ClarityCostFunction and CostTracker if you think that would be cleaner. Let me know what you prefer!
| pub type CheckResult<T> = Result<T, CheckError>; | ||
|
|
||
| #[derive(Debug, PartialEq)] | ||
| pub enum CheckErrors { |
There was a problem hiding this comment.
would it be consensus breaking if we modify CheckErrors to Box all TypeSignature and Value in order to remove #![allow(clippy::result_large_err)]?
There was a problem hiding this comment.
I think that should be fine, because we don't ever check a type's substructure to see if it contains a Box<T>.
There was a problem hiding this comment.
Given it will be require a big amount of line changes, I'll just do it in a separate PR
There was a problem hiding this comment.
Just for completeness, here is the PR that boxes the data in some errors and fixes all the clippy::result_large_err warnings. No need to review it now of course :)
There was a problem hiding this comment.
We've opened a PR as a follow-up to expand input coverage further, in line with #6355.
Any feedback/thoughts? |
Description
Points for Discussion from #6297:
clarity-serialization, I consolidated all error types into a single CodecError enum with a 1:1 match with the original errors inclarity. However, when converting back to the older error types usingFrom<CodecError>, some variants don’t have a clear or meaningful equivalent. I’ve routed them through generic catch-all cases. Even if the current code doesn't reach those conversions, I am not happy with this mapping strategy and am open to suggestions!Applicable issues
Additional info (benefits, drawbacks, caveats)
Checklist
docs/rpc/openapi.yamlandrpc-endpoints.mdfor v2 endpoints,event-dispatcher.mdfor new events)clarity-benchmarkingrepobitcoin-tests.yml